-
-
Notifications
You must be signed in to change notification settings - Fork 72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Consents for training requests #2363
Consents for training requests #2363
Conversation
It contains common fields and methods used in descendant classes. New descendant to be introduced in next commit.
…ing consent fields This solution is similar to the approach from BaseTermConsentsForm.
7a97b14
to
11e94f8
Compare
Ready for your review, @elichad! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, realised I never submitted my review comments from yesterday. I'm happy with this, just have a couple of questions.
@@ -963,6 +952,18 @@ def trainingrequests_merge(request): | |||
"obj_a": obj_a, | |||
"obj_b": obj_b, | |||
"form": form, | |||
"obj_a_consents": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can include this in person_merge
view as well - currently the new-style consents don't show up there. I've made an issue for that #2370
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll complete this as a separate ticket since there's a depending PR #2375.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: #2376
return False | ||
# Create new style consents based on the training request consents used in the | ||
# training request form. | ||
training_request_consents = TrainingRequestConsent.objects.filter( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After this change/migration, we will have some training requests in the database which don't have any TrainingRequestConsents set. Am I right in understanding that if a Person is created from such a request, they will instead provide these consents when they first log in to AMY?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side comment - I haven't actually been able to test the creation of a Person from a training request with this PR. Locally I get an error:
BadCredentialsException at /requests/training_request/41/
401 {"message": "Bad credentials", "documentation_url": "https://docs.github.com/rest"}
I assume this is because I don't have GH auth in my local instance. And I assume it won't work on test-amy for the same reason? Is there some way we can double check this works in the UI before releasing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After this change/migration, we will have some training requests in the database which don't have any TrainingRequestConsents set. Am I right in understanding that if a Person is created from such a request, they will instead provide these consents when they first log in to AMY?
I have just tested this and you are correct.
Locally I get an error:
Yes, this is because it's trying to confirm GH handle with GH API. You can erase the handle from request by editing it and you should be able to continue.
You can also create a token for your own use here: https://github.com/settings/tokens
Use AMY_GITHUB_API_TOKEN
envvar to set the token.
@@ -141,13 +141,25 @@ | |||
— | |||
{% endif %} | |||
</td></tr> | |||
<tr><th>Data privacy agreement:</th> | |||
<tr><th><span class="badge badge-secondary">(Deprecated)</span> Data privacy agreement:</th> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At what point would we remove this line altogether (if ever)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know. Perhaps we should keep this for now.
This fixes #2162.
To-do:
TrainingRequestConsent
when training request is createdTrainingRequestConsent
s asConsent
s when matching training request with a person